Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc,http2: add parameters for Http2Session:connect event #20193

Closed
wants to merge 1 commit into from

Conversation

ryzokuken
Copy link
Contributor

Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

Checklist

/cc @mcollina @nodejs/http2

@ryzokuken ryzokuken self-assigned this Apr 21, 2018
@ryzokuken ryzokuken requested a review from mcollina April 21, 2018 05:40
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Apr 21, 2018
@Trott
Copy link
Member

Trott commented Apr 21, 2018

We don't typically list parameters like that for events, do we? I think it's useful information to have, but I'm not sure that's the best way to list it. (Maybe it is. I honestly don't know.)

@nodejs/documentation

@apapirovski
Copy link
Member

@Trott we do in the http and streams documentation. For example: https://nodejs.org/dist/latest-v9.x/docs/api/http.html#http_event_connect

@Trott
Copy link
Member

Trott commented Apr 21, 2018

@Trott we do in the http and streams documentation. For example: https://nodejs.org/dist/latest-v9.x/docs/api/http.html#http_event_connect

I never noticed that. OK, cool. If it's what we already do elsewhere, at least it's a unified approach.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the "The associated ... instance" is that helpful since the same information is conveyed by the type annotation already. Otherwise, LGTM.

doc/api/http2.md Outdated
@@ -136,6 +136,9 @@ listener does not expect any arguments.
added: v8.4.0
-->

* `session` {EventEmitter} The current `Http2Session` instance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use {Http2Session} here.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with @TimothyGu nit.

@ryzokuken
Copy link
Contributor Author

@TimothyGu @mcollina is this better?

doc/api/http2.md Outdated
@@ -136,6 +136,9 @@ listener does not expect any arguments.
added: v8.4.0
-->

* {Http2Session}
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit unclear what this line means. If this is the first parameter of an event listener, we need to add a name to it, as the only unnamed types in docs are types of properties and returned values. If this is an event emitter, I do not think we formally state them in this way, as this is already inferred from the section place; this can baffle doctools.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm, so should I add a name to it or skip it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is not a listener parameter, but an event source, I think we can skip it as this event is already a subsection of Http2Session section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a parameter. Code:

process.nextTick(emit, this, 'connect', this, socket);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need to add a name then for consistency if I am not mistaken. It seems we normally have not unnamed parameters in docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming it session, in that case.

@ryzokuken
Copy link
Contributor Author

This is a single documentation change, and I believe it should be fast-forwarded. Please respond with 👍 to approve fast-forwarding this.

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 21, 2018
@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 21, 2018
@ryzokuken
Copy link
Contributor Author

CI passed! 🎉
Landing it tonight.

Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)
@ryzokuken
Copy link
Contributor Author

Squashing.

@ryzokuken
Copy link
Contributor Author

Re-running CI Lite.

CI Lite: https://ci.nodejs.org/job/node-test-pull-request-lite/568/

@ryzokuken
Copy link
Contributor Author

Passed again, landing.

@ryzokuken
Copy link
Contributor Author

Landed in 31cbec7

@ryzokuken ryzokuken closed this Apr 21, 2018
ryzokuken added a commit that referenced this pull request Apr 21, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20193
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20193
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: nodejs#20193
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Sep 11, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: nodejs#20193
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Sep 19, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: nodejs#20193
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: nodejs#20193
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

Backport-PR-URL: #22850
PR-URL: #20193
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants